Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: multi screen window show #450

Open
wants to merge 13 commits into
base: dev
Choose a base branch
from
Open

fix: multi screen window show #450

wants to merge 13 commits into from
+61 −3

Conversation

AkaShark
Copy link
Collaborator

@AkaShark AkaShark commented Mar 10, 2024 •
edited
Loading

closed #442

Add a picker in the general page to display the window as auto or fixed when translation finishes.

phlpsong
phlpsong previously approved these changes Mar 10, 2024
View reviewed changes
Copy link
Collaborator

@phlpsong phlpsong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

tisfeng
tisfeng reviewed Mar 11, 2024
View reviewed changes
Easydict/Feature/Utility/EZCoordinateUtils/EZCoordinateUtils.m Outdated
if (Configuration.shared.showWindowMultiScreen == EZShowWindowMultiScreenAuto) {
screen = [self screenOfMousePosition];
} else {
screen = NSScreen.mainScreen; // EZShowWindowMultiScreenFixed fixed in main screen
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it should not use mainScreen here, it should use the screen when selecting words.

tisfeng
tisfeng requested changes Mar 11, 2024
View reviewed changes
Copy link
Owner

@tisfeng tisfeng left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally, the position of the word is basically the same as the mouse position, but if the user moves the mouse position quickly during the query, or even across multiple screens, the problem will be complicated, which I have not considered before.

I thought about it and felt that there is no need to add an additional option here. We should use the screen where the selected word is located, rather than the current mouse position.

Adding an extra setting option may confuse users. Using the location of word selection aligns better with user intuition.

@AkaShark
Copy link
Collaborator Author

okay I will fix it later

@AkaShark AkaShark requested review from tisfeng and phlpsong March 12, 2024 04:58
@AkaShark
Copy link
Collaborator Author

AkaShark commented Mar 12, 2024 •
edited
Loading

I remove the picker from the settings page. When the user starts the query action I record the mouse-located screen and remove this record when the floating window closes.

tisfeng
tisfeng requested changes Mar 12, 2024
View reviewed changes
Copy link
Owner

@tisfeng tisfeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it, but the current code does not work #442 (comment) :

Get the word on screen A, then move the mouse to screen B, and then use the shortcut keys to translate the word. The query window should be displayed at the word pick-up position on screen A.

@tisfeng
Copy link
Owner

tisfeng commented Mar 12, 2024

This problem can be a bit complex, involving multiple screens of records and the handling of the location of the display query window.

If you're not good at solving it, I can take over later.

@AkaShark
Copy link
Collaborator Author

Oh, I now understand your issue. I didn't consider issue #442 earlier and only focused on the mouse move duration during query translation. To address this, I will record the screen when starting a query like this

- (void)startQueryText:(NSString *)text actionType:(EZActionType)actionType {
    // record current screen when start query
    NSScreen *current = [EZCoordinateUtils screenForPoint: [NSEvent mouseLocation]];
    EZCoordinateUtils.startQueryScreen = current;

Let me try it first. I'll ask for your help if I find it difficult for me.

@AkaShark
Copy link
Collaborator Author

@tisfeng My idea is to record the screen when a word is selected for translation and display a query window on this screen. For other modes of translation, the screen will still be recorded when the query is initiated. What do you think about it?

@tisfeng
Copy link
Owner

tisfeng commented Mar 12, 2024

I'm not so sure, you can try it and then we'll test it.

@AkaShark
Copy link
Collaborator Author

I'm not so sure, you can try it and then we'll test it.

okk

@AkaShark AkaShark requested a review from tisfeng March 13, 2024 15:40
tisfeng
tisfeng requested changes Mar 14, 2024
View reviewed changes
Copy link
Owner

@tisfeng tisfeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still doesn't work.

Although the latest code can record screen A #450 (review) , the position of the query window displayed when using the shortcut key to stroke the words to translate is inaccurate, and there is a problem that the query window flickers between screen A and screen B.

It's not easy for me to record a video to show you this problem, I suggest you test it yourself using multiple screens, its very easy to reproduce.

@tisfeng
Copy link
Owner

tisfeng commented Jun 7, 2024

@AkaShark When you have time, please continue pushing this PR.

@tisfeng tisfeng mentioned this pull request Jun 8, 2024
Open
6 tasks
@AkaShark
Copy link
Collaborator Author

AkaShark commented Jun 8, 2024

@AkaShark When you have time, please continue pushing this PR.

okay, I will try to fix it in the last few days

@AkaShark
Copy link
Collaborator Author

I'm sorry I've been a little busy lately. I'll finish this issue as soon as possible.

@tisfeng
Copy link
Owner

tisfeng commented Jun 24, 2024

ok, you can schedule as you see fit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tisfeng tisfeng tisfeng requested changes

@NeverAgain11 NeverAgain11 Awaiting requested review from NeverAgain11

@choykarl choykarl Awaiting requested review from choykarl

@CanglongCl CanglongCl Awaiting requested review from CanglongCl

@Jerry23011 Jerry23011 Awaiting requested review from Jerry23011

@phlpsong phlpsong Awaiting requested review from phlpsong

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AkaShark @tisfeng @phlpsong